Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibWeb: ShadowRealms part 4 - enough for it to not crash #1993

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

shannonbooth
Copy link
Contributor

@shannonbooth shannonbooth commented Oct 27, 2024

Sitting ontop of #2134

This implements enough for the test case:

<script>
async function test() {
    const realm = new ShadowRealm()
    const aSync = realm.evaluate(`async () => 1 + 1`)
    aSync()
}

test();
</script>

To work.

Unfortunately, WPT test cases are not passing as there is something going wrong with importing scripts (meaning that the test harness is not set up correctly), but this is still progress forwards!

http://wpt.live/url/idlharness-shadowrealm.window.html s

@shannonbooth shannonbooth changed the title LibWeb: ShadowRealms part 3 - enough to get it mostly working LibWeb: ShadowRealms part 4 - enough to get it mostly working Oct 27, 2024
@shannonbooth shannonbooth force-pushed the shadow-realm-mvp branch 7 times, most recently from a16bd7c to 2b099fa Compare November 4, 2024 15:02
@shannonbooth shannonbooth changed the title LibWeb: ShadowRealms part 4 - enough to get it mostly working LibWeb: ShadowRealms part 4 - enough for it to not crash Nov 4, 2024
@shannonbooth shannonbooth marked this pull request as ready for review November 4, 2024 15:05
With the introduction of shadow realms, there will be two different
possible host defined objects. For clarity, rename the existing host
defined object to PrincipalHostDefined.
This class is the host defined field of a synthetic realm created as
part of a shadow realm.
This object represents the global object for a shadow realm. The IDL
generator will need to be adjusted to the '[Global]' extended attribute
and no '[Exposed]' field (the change in the test is not correct, as I
understand it), but this should be enough to get us started on
shadow realms.
This is enough for a basic shadow realm to work :^)

There is more that we still need to implement here such as module
loading and fixing up the global object, but this is enough to get some
basic usage working.
Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp Outdated Show resolved Hide resolved

// 11. Perform ? SetDefaultGlobalBindings(realm).
set_default_global_bindings(realm);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should call ShadowRealmGlobalScopeGlobalMixin::initialize(realm, *this); here, as well as add_shadow_realm_exposed_interfaces(*this) (in a helper on the global scope/global object variable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

speaking of which, did the spec folks decide what things should be exposed to shadow realms? We'll need to go around sprinkling annotations all over our idl files for the idl generator to pick them up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, spec folks have done so - those are all of the crashing IDL tests that we have currently. Here's a meta issue that shows what has been done: tc39/proposal-shadowrealm#393

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not crashing after these changes, but still doesn't work :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah perfect, then it should be straightforward to add ShadowRealm as an option in GenerateWindowOrWorkerInterfaces ... which I guess should be renamed to GenerateGlobalExposedInterfaces or some such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, yeah that's why I've left those other FIXME's you mentioned in the initialize steps. I need to implement that universal global scope and reshuffling some implementations from windoworworkerglobal scope mixin to a new universalglobalscope mixin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha. well, given that, I think we're good to go on this part once CI is happy. Feel free to add more fixmes if you want though.

@ADKaster ADKaster merged commit 9598ed1 into LadybirdBrowser:master Nov 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants